fix: depth calculation when add previous events#275
Conversation
WalkthroughUpdates addPrevEvents to compute depth using the maximum predecessor depth rather than list order. Adds a new test verifying depth recalculation with unordered predecessors and multiple insertions. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Event as EventWrapper
participant Store as Prev Events
Caller->>Event: addPrevEvents(events[])
Event->>Store: Read depths of predecessors
Note over Event: Compute deepestDepth = max(depths)
alt deepestDepth >= current depth
Event->>Event: rawEvent.depth = deepestDepth + 1
else
Event->>Event: Keep current depth
end
Event-->>Caller: return
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #275 +/- ##
==========================================
+ Coverage 60.30% 60.46% +0.16%
==========================================
Files 67 67
Lines 6673 6673
==========================================
+ Hits 4024 4035 +11
+ Misses 2649 2638 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/room/src/manager/event-wrapper.ts (2)
458-459: Refactor to avoid array mutation and improve performance.The current implementation sorts the input array in place and uses
pop()to find the maximum depth. This has two issues:
- It mutates the caller's array, which is an unexpected side effect
- Sorting is O(n log n) when finding the maximum is O(n)
Apply this diff to use
Math.maxinstead:- const deepestDepth = - events.sort((e1, e2) => e1.depth - e2.depth).pop()?.depth ?? 0; + const deepestDepth = events.length > 0 + ? Math.max(...events.map(e => e.depth)) + : 0;Alternatively, for a more concise version:
- const deepestDepth = - events.sort((e1, e2) => e1.depth - e2.depth).pop()?.depth ?? 0; + const deepestDepth = Math.max(0, ...events.map(e => e.depth));
460-462: Consider adding a clarifying comment.The conditional depth update using
<=is correct for supporting incrementaladdPrevEventscalls, where depth should increase with each batch of predecessors but never decrease. However, the reasoning might not be immediately obvious to future readers.Consider adding a brief comment:
+ // Update depth if any predecessor is deeper or equal (allows incremental predecessor additions) if (this.rawEvent.depth <= deepestDepth) { this.rawEvent.depth = deepestDepth + 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/room/src/manager/event-wrapper.spec.ts(2 hunks)packages/room/src/manager/event-wrapper.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/room/src/manager/event-wrapper.spec.ts (2)
packages/room/src/manager/factory.ts (1)
PersistentEventFactory(30-123)packages/room/src/types/v3-11.ts (1)
Pdu(769-769)
🔇 Additional comments (2)
packages/room/src/manager/event-wrapper.spec.ts (2)
4-4: LGTM!The import of the
Pdutype is appropriate for type casting in the test cases.
371-399: Excellent test coverage!This test case thoroughly validates the depth calculation logic:
- Verifies depth increases correctly with each
addPrevEventscall- Tests the cumulative effect of multiple predecessor additions (1 → 2 → 6 → 8)
- Validates order-independence by intentionally passing predecessors out of order (e5, e4)
The test confirms that depth is correctly calculated as
max(predecessor_depths) + 1regardless of input order.
Summary by CodeRabbit
Bug Fixes
Tests